-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Simplify inventory client parsing #963
base: master
Are you sure you want to change the base?
Conversation
In general I'm not a fan of the former implementation because it coerces an object into a string in order to achieve address normalisation into checksum case, so I opted to operate directly on the string here. |
f0d582d
to
4656e88
Compare
@@ -65,11 +65,12 @@ export class RelayerConfig extends CommonConfig { | |||
this.slowDepositors = SLOW_DEPOSITORS | |||
? JSON.parse(SLOW_DEPOSITORS).map((depositor) => ethers.utils.getAddress(depositor)) | |||
: []; | |||
this.inventoryConfig = RELAYER_INVENTORY_CONFIG ? JSON.parse(RELAYER_INVENTORY_CONFIG) : {}; | |||
this.inventoryConfig = JSON.parse( | |||
RELAYER_INVENTORY_CONFIG?.replace(/0x[a-fA-F0-9]{40}/g, ethers.utils.getAddress) ?? "{}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should make this a helper function? I think there are several other places where we expect an address string set in the .env that might not be checksummed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can make it a utility function; I only did it here because this was the only place I could see that we actually normalised addressed on input.
I'll update accordingly 👍
@@ -65,11 +65,12 @@ export class RelayerConfig extends CommonConfig { | |||
this.slowDepositors = SLOW_DEPOSITORS | |||
? JSON.parse(SLOW_DEPOSITORS).map((depositor) => ethers.utils.getAddress(depositor)) | |||
: []; | |||
this.inventoryConfig = RELAYER_INVENTORY_CONFIG ? JSON.parse(RELAYER_INVENTORY_CONFIG) : {}; | |||
this.inventoryConfig = JSON.parse( | |||
RELAYER_INVENTORY_CONFIG?.replace(/0x[a-fA-F0-9]{40}/g, ethers.utils.getAddress) ?? "{}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be easier to use a reviver in this case. JSON.parse
's second parameter is an optional function that looks at keys/values. We could probably create a reviver function like:
/**
* Converts a potential string/array of strings that match an address into a checksummed address
**/
export function checksumAddress(value: unknown): unknown {
const isArray = Array.isArray(value);
const result: unknown[] = (isArray ? value : [value]).map((v) => utils.isAddress(v) ? utils.getAddress(v) : v;
return isArray ? result : result[0];
}
JSON.parse(
RELAYER_INVENTORY_CONFIG ?? {},
(, v) => checksumAddress(v)
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment to possibly add discussion about the readability of the parse function
This helps to drop an UMA dependency, and is arguably nicer because it doesn't corce the js Object to and from a string type.
4656e88
to
b719736
Compare
This helps to drop an UMA dependency, and is arguably nicer because it doesn't corce the js Object to and from a string type.